-
Notifications
You must be signed in to change notification settings - Fork 39
[최재호] sprint4 #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[최재호] sprint4 #126
The head ref may contain hidden characters: "Basic-\uCD5C\uC7AC\uD638-sprint4"
Conversation
addiescode-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
하나의 함수가 너무 많은 일을 하지않도록 작업 단위를 쪼개는것부터 리팩토링 시작해보시면 좋을것같아요. 단계별로 어떻게 리팩토링하면될지 error.js에 자세히 예시를 들어드렸으니 참고해보세요!
주요 리뷰 포인트
- 유지보수를 고려한 개발
| @@ -0,0 +1,106 @@ | |||
| export function ErrorCheck(code, data = "") { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반적으로 함수 네이밍은 파스칼케이스를 사용하지않고, 동사로 시작합니다 :)
| export function ErrorCheck(code, data = "") { | |
| export function checkError(code, data = "") { |
아래 아티클 참고해보시면 도움이 되실것같아요!
참고
|
|
||
| const inputBorder = "2px solid red" | ||
|
|
||
| if (code == "email") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if문 중첩이 있어서 가독성이 많이 떨어지고, 하나의 함수가 관여하는일이 너무 많아서 유지보수가 어려워보입니다.
이 두가지를 해결하기위해서는 함수가 단일 책임을 가질수있도록 분리해보시는게 좋습니다.
우선 이런 방식은 어떨까요?
// 공통 스타일 정의
const errorStyle = {
color: "red",
fontSize: "20px",
marginTop: "1px",
display: "block",
};
const successStyle = {
color: "",
fontSize: "",
marginTop: "",
};
const errorBorder = "2px solid red";
const successBorder = "";
// 유효성 검사 함수들
const validateEmail = (email) => {
if (!email) {
return {
error: true,
message: "이메일을 입력해주세요",
};
}
const regex = /^((?!\.)[\w\-_.]*[^.])(@\w+)(\.\w+(\.\w+)?[^.\W])$/;
const isValid = regex.test(email);
return {
error: !isValid,
message: isValid ? "" : "잘못된 이메일 형식 입니다.",
};
};
const validatePassword = (password) => {
if (!password) {
return {
error: true,
message: "비밀번호를 입력해주세요.",
};
}
const isValid = password.length > 8;
return {
error: !isValid,
message: isValid ? "" : "비밀번호를 8자 이상 입력해주세요.",
};
};
const validateNickname = (nickname) => {
const isValid = nickname.length >= 1;
return {
error: !isValid,
message: isValid ? "" : "닉네임을 입력해주세요.",
};
};
const validatePasswordCheck = (passwords) => {
const isValid = passwords.password1 === passwords.password2;
return {
error: !isValid,
message: isValid ? "" : "비밀번호가 일치하지 않습니다",
};
};
// 메인 함수
export function checkError(code, data = "") {
let validationResult;
switch (code) {
case "email":
validationResult = validateEmail(data);
break;
case "password":
validationResult = validatePassword(data);
break;
case "nickname":
validationResult = validateNickname(data);
break;
case "passwordCheck":
validationResult = validatePasswordCheck(data);
break;
default:
return {
error: false,
message: "",
messageStyle: successStyle,
inputBorder: successBorder,
};
}
return {
error: validationResult.error,
message: validationResult.message,
messageStyle: validationResult.error ? errorStyle : successStyle,
inputBorder: validationResult.error ? errorBorder : successBorder,
};
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 함수가 한번에 하나의 일을 할 수 있게끔 쪼개고 보니,
유효성 검사를 수행할때도 각각 따로 함수를 만들 필요가 없어보이고, 재사용 가능한 단위로 분리하는것이 가능해보입니다.
그렇다면 유효성 검사를 수행할때 재사용을 고려해 좀 더 통합적으로 관리될수있도록 만들어볼까요?
코드 예시를 들어드릴게요!
- 유효성 검사를 담당하는 validation.js
// validation.js - 검증 로직
export const validators = {
email: (value) => {
if (!value) return { isValid: false, message: '이메일을 입력해주세요' };
if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) {
return { isValid: false, message: '올바른 이메일 형식이 아닙니다' };
}
return { isValid: true, message: '' };
},
nickname: (value) => {
if (!value) return { isValid: false, message: '닉네임을 입력해주세요' };
return { isValid: true, message: '' };
},
password: (value) => {
if (!value) return { isValid: false, message: '비밀번호를 입력해주세요' };
if (value.length < 8) {
return { isValid: false, message: '비밀번호는 8자 이상이어야 합니다' };
}
return { isValid: true, message: '' };
},
passwordCheck: (value, formValues) => {
if (!value) return { isValid: true, message: '' }; // 빈 값은 허용
if (value !== formValues.password) {
return { isValid: false, message: '비밀번호가 일치하지 않습니다' };
}
return { isValid: true, message: '' };
}
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 방식은
이렇게 validation을 통합적으로 담당하는 객체를 만들고, checkError 함수에서는 이런식으로 사용해준다면
export function checkError(type, data = "") {
const validator = validators[type];
if (!validator) {
return {
error: false,
message: "",
messageStyle: successStyle,
inputBorder: successBorder,
};
}
const result = validator(data);
return {
error: !result.isValid,
message: result.message,
messageStyle: !result.isValid ? errorStyle : successStyle,
inputBorder: !result.isValid ? errorBorder : successBorder,
};
}개별적으로 다른 유효성 검사를 수행하던 함수들을 없애주고, switch case문도 사용하지않게 되니까 코드양이 훨씬 줄어들고, 읽기 수월해지고, 수정 및 확장에 용이해지고, 코드가 간결해지겠죠?
|
|
||
| passwordLabel.parentNode.insertBefore(warningMessage1, passwordLabel) | ||
|
|
||
| emailInput.addEventListener("focusout", (e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수도 위에서 얘기했던것처럼 하나의 함수가 너무 많은 일을 담당하고있어요.
- validation 상태에 따른 UI 업데이트하기
- 입력 필드 초기 상태 설정하기
- 이벤트 핸들러 연결하기
세가지 함수로 작업을 쪼개서 리팩토링해볼까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서도 login.js에서 드린 코멘트 참고해주세요 :)
요구사항
기본
로그인
회원가입
심화
스크린샷
웹사이트 링크
Link
멘토에게
@addiescode-sj